Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code refresh + SPM support #848

Merged
merged 15 commits into from
Jul 26, 2023
Merged

Conversation

Climbatize
Copy link
Contributor

@Climbatize Climbatize commented Nov 8, 2022

This PR is used in production since the last 9 month with no issues so far. (through this SPM dependency: https://github.com/Climbatize/XLPagerTabStrip.git > 9.1.0)

  • Updated source code to remove warnings and use clang instead of GCC.
  • Upgraded support to iOS 11 and added support for SPM.
  • Moved FXPageControl to a SPM dependency
  • Fixed deprecated code concerning window orientation.

I know there is already this good PR #846 aiming to bring support to SPM, but I think this lib needed a deeper refresh to remove some deprecations.

Copy link
Member

@mats-claassen mats-claassen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Climbatize thank you for contributing and for the heads up. Sorry for the delay. I left just two comments on the code changes. Did you also test installing these changes through Cocoapods?

Comment on lines 29 to 31
@IBOutlet weak var userImage: UIImageView!
@IBOutlet weak var postName: UILabel!
@IBOutlet weak var postText: UILabel!
@IBOutlet var userImage: UIImageView!
@IBOutlet var postName: UILabel!
@IBOutlet var postText: UILabel!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding making the IBOutlets strong, could you point me to documentation stating that weak is not recommended? I read the opposite in this doc.
Also connecting an Outlet from Xcode defines it as weak by default

Copy link
Contributor Author

@Climbatize Climbatize Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit Apple documentation is confusing. So I remade the IBOutlets from XCode just to confirm. Also, doc specifies The strong outlets are frequently specified by framework classes (for example, UIViewController’s view outlet, or NSWindowController’s window outlet), that's why I did this change. I honestly think it's more dangerous to have it as weak than strong, as I already experienced crashes related to weak outlets in production, but I can revert if you think it's better.

Screen.Recording.2023-07-26.at.9.07.59.mov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen both happen by default. For example when you build a UIViewController and want to connect its view to the controller class it defaults to strong (as that view should be strong). But for other views which are subviews of the main view it will default to weak (because subviews will be strongly referenced by their parent). This means as long as a view is in the view hierarchy it should not be deallocated and you should have no issue. However if you plan to remove the view you will have to strongly reference it if you plan to continue using it afterwards.

I think it is best to revert unless there are reproducible crashes which I would rather think could be linked to how the views are used. It might make sense to switch some of the outlets to strong if the use cases require it (rather than just switching everything)

import FXPageControl
#endif

extension UIWindow {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this extension should be moved to a separate file named something like UIWindow+Pager.swift or so

@@ -1,7 +1,7 @@
//
// FXPageControl.h
//
// Version 1.4
// Version 1.5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this file out of the Sources folder will make it not be included in the library when installed through Cocoapods and it is also not marked as dependency. Please either revert or add FXPageControl as dependency in the podspec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a big mistake from my side! I ran a pod spec lint XLPagerTabStrip.podspec instead of pod lib lint XLPagerTabStrip.podspec. Fix is on the way

Moved UIWindow extension to an independant file, updated podspec to take objective C files refactoring in account
@Climbatize
Copy link
Contributor Author

Climbatize commented Jul 26, 2023

Thanks @mats-claassen for the quick and kind review! I left my comments and provided update based on your feedbacks.

@mats-claassen
Copy link
Member

@Climbatize changes look good. Just the matter of the weak outlets remaining

@Climbatize
Copy link
Contributor Author

I reverted changes on weak outlets. At first sight no crashes from my side. I think we're good to merge @mats-claassen :)

@mats-claassen mats-claassen merged commit 211ed62 into xmartlabs:master Jul 26, 2023
@Climbatize
Copy link
Contributor Author

Climbatize commented Jul 27, 2023

Thanks for the merge @mats-claassen ! Don't forget to tag the master branch to a minor version greater than 9.0.0 so the release is accessible, or to edit the the readme file to tell devs to point to master branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants